Still thinking through this bit - by taking the service by value, I think we implicitly impose the requirement that either we take sole ownership of the service, or the service's control handle must be cloneable.

In the case of the time-alarm service I think it's fine for the control handle to be cloneable, and that's in line with what e.g. embassy_net::Stack does, but that may not generalize well to other services. I think it carries the implication that the service's internal reference must be non-mut, which is true of the time-alarm service but may not be for other services that want external synchronization.

Maybe the answer here is that for services that want to be relayable and also have methods that take &mut self, their relay handler implementations need to be generic over a Lockable or something? Would be interested in others' thoughts on this one

So, I'm guessing the issue with consuming the service by value and taking ownership is the possibility we may want multiple wrappers similar to the relay handler wrapper in the future, wrapping the same service simultaneously?

For interfaces that only take &self, another option is we could just blanket impl TimeAlarmService for &T so at least callers have the option if they want the relay service to own T or share it with others.

So far, I think relayable services at least only use methods that take &self in their implementations of RelayServiceHandler. But I agree I think it makes sense to let the service decide how it wants to handle this, whether with a generic Lockable or whatever, if it needs &mut self for its RelayServiceHandler implementation.

Yeah, more generally it's that we may want to share access to a service between multiple clients, of which a relay handler is one.

In the case of the time-alarm service, you need a handle to the service to be able to query the RTC for wall-clock time, so any service that wants to be able to do that needs to also get a handle to it somehow. For a system-wide service like the time-alarm service, this is probably fine; most time APIs I'm aware of work this way (where you don't need an exclusive handle to the clock to read the time). That implies that the service is using interior mutability via a mutex or something, and in those cases, we can just make the handle cloneable and give everyone a handle. That's what I've done here, and it's in line with what embassy_net::Stack does.

This falls apart if the interface has any &mut methods on it, though, which I think is more likely on things that we may not want to share / want to use exterior mutability on. The cyw43 driver is a good example for this - you need to use a &mut reference to e.g. join a wifi network. I know Surface has expressed interest in doing this on some classes of services (USB-C comes to mind), so I want to make sure we have a path to enabling those use cases too. I think it might involve making the relay agent generic over a Lockable<ServiceHandle> or something?

I think you're right that all the currently relayable services are only taking &self rather than &mut self, but I think in some cases that may be vestigial from the comms system, where locking had to be the responsibility of the service because the comms system itself needed a reference to the service to be able to deliver messages. Do you know if there are any methods on relayable services that we may want to move to taking &mut self? If not, maybe we punt on demonstrating the lockable thing for now - I'm pretty sure it'd work, it's just a bit verbose (because in addition to putting the Resources object in a static / StaticCell, you also have to put the control handle in a static StaticCell<Mutex<ControlHandle>> or something. That may be unavoidable if we want &mut methods on the interface trait, though?

@asasine @JamesHuard @RobertZ2011 thoughts on this approach?

Could you clarify which part you mean should be generic over a Lockable? Maybe I'm a bit confused where exactly you see that (in RelayHandler? RelayServiceHandler?)

I think I'm generally in agreement that it's better to leave the service Consumer here as the owner and take it by value/a move:

fn new(s: impl ServiceInterface) -> Self { .. }

And then if a trait can be impl<T: ServiceInterface> ServiceInterface for &T { .. }.

For when you need shared mutation across multiple owners of the same exact instance, I think there's multiple options here and they each have their own trade-offs (and applications).

One such set of shared impl ServiceInterface would be those that are "simply lockable". Obvious examples would be things like device drivers or shareable buses. In this case, a generic abstracted type like MutexShared or Lockable would work well:

// abstracted interface crate
pub trait ServiceInterface { .. }

// if `ServiceInterface` does not require `mut`
impl<T: SeviceInterface> ServiceInterface for &T { .. }

// perhaps simplest way for this particular use case, but not necessarily the best approach. Reasons why is the exact semantics around the mutex accessor here could be dependent heavily on T's internals -- so probably better to put this next to the "Provider" that impl's ServiceInterface
impl<M: RawMutex, T: ServiceInterface> ServiceInterface for Mutex<M, T> { .. }

// some device driver that impl's the interface crate
pub struct ServiceDevice { .. }
impl service_interfaces::ServiceInterface for ServiceDevice { .. }
// probably more idiomatic than 'blanket' impl in interfaces crate
// technically this may need to be a new-type LockedServiceDevice, omitting for brevity
impl<M> service_interfaces::ServiceInterface for Mutex<M, ServiceDevice> { .. }

However, there might be scenarios where you actually care about the order in which events occur. In those cases, you'd need some kind of serializer of requests, which a Mutex can't guarantee because it's lock().await will have yield behaviors dependent on the scheduler.

In that case, one straight-forward option would be to have a single owner "task" for your impl ServiceInterface. Then have a channel for propagating requests, and impl ServiceInterface for a new-type over said channel. Roughly,

// in a separate 'service-event-servicer/provider' crate
// _ignoscerus meus propter nominus_
pub struct EventServicer { .. }
impl service_interfaces::ServiceInterface for EventServicer { .. }

// maybe some helper type here to provide an off-the-shelf impl
// this could easily be a Send:Response pattern if needed
pub enum Dispatch { .. }
pub struct SerializedEventServicer<M, const N: usize>(Channel<M, Dispatch, N);

impl<M: RawMutex, const N: usize> SerializedEventServicer {
    fn provider(&self) -> SerializedEventProvider<M> {
        SerializedEventProvider(self.0.sender().into())
    }
}

// the real "magic"
pub struct SerializedEventProvider<M>(SendDynSender<M, Dispatch>);
impl service_interfaces::ServiceInterface for SerializedEventProvider { .. }

Then as a consumer of the crate, I can use the 'efficient' direct ownership impl ServiceInterface via EventServicer if I only have one ServiceInterface consumer. But if I have multiple, I can evoke a shared (but correctly serialized) impl ServiceInterface via SerializedEventProvider.

In a more general sense, I would be weary of doing anything that looks like:

pub struct OwnedRefServiceConsumer<'a, S: service_interfaces::ServiceInterface> {
    service_owner: &'a S
}

As I think that pattern is too restrictive and blocks arbitrary composition of impl ServiceInterface and Consumer<S: ServiceInterface>. I think it might be OK for a helper type in some specific scenarios -- but that would be at the impl ServiceInterface's discretion, not the Consumer<impl ServiceInterface>.

I think that distinction is important.

Got it, though RelayServiceHandler isn't currently generic over a impl FooService either so still not entirely sure where this will go. But I'm probably missing something so I'll think about it some more.

Edit: This was in response to Billy's comment above

Got it, though RelayServiceHandler isn't currently generic over a impl FooService either so still not entirely sure where this will go. But I'm probably missing something so I'll think about it some more.

Sorry, to clarify, I'm talking about this bit:
impl<T: TimeAlarmService> TimeAlarmServiceRelayHandler<T>

in this one, the TimeAlarmServiceRelayHandler is generic over T: TimeAlarmService - I'm saying that in the case where the handle that the TimeAlarmServiceRelayHandler has &mut self methods on it, you won't be able to just pass a reference to it into the relay handler, you'll need some other synchronization to access the &mut self methods, and the ergonomics there are unpleasant; you need to put the Resources into a static and then also put the control handle into a static Mutex<ControlHandle> or something, which ends with you spelling a potentially-complex type a bunch of times. I think that may be unavoidable, though, and it can kind of be mitigated by using type aliases?

Sounds like consensus is to proceed with the owned relay handler so I'll do that and we'll just accept that we may need to do the mutex thing. Probably won't come up for the four currently-relayable services unless we decide we want to make some of those methods take &mut self, but I think we can punt that to a future change since this one is already going to be moving a lot of code into other files and reviewing is probably going to be tricky enough without also reshaping the existing interfaces

Probably won't come up for the four currently-relayable services unless we decide we want to make some of those methods take &mut self

With the &mut self in the methods for given ServiceInterface, isn't that also solved for by the mutex case by just impl ServiceInterface for &Mutex<Thing> { .. }? (Or some newtype over ref to Mutex)?

Probably won't come up for the four currently-relayable services unless we decide we want to make some of those methods take &mut self

With the &mut self in the methods for given ServiceInterface, isn't that also solved for by the mutex case by just impl ServiceInterface for &Mutex<Thing> { .. }? (Or some newtype over ref to Mutex)?

That sounds right to me. Or to be even more generic, can do a blanket impl<L: Lockable> TimeAlarmService for L where L::Inner: TimeAlarmService